[feat]: add experimental WorkflowUI telemetry hooks #221

jamieQ · 2023-06-02T15:43:00Z

Motivation

we'd like to be able to build out some observation tooling that is aware of certain events that occur within WorkflowUI. specifically, view lifecycle data is of particular interest. currently the options for tying into UIViewController lifecycle events are either:

introduce new subclasses in client code that override the methods of interest and provide observation hooks there.
swizzle the methods of interest to allow client observation

option 1. may place a large burden on consumers that have many existing subclasses (of, say, ScreenViewController), and only works for the open types defined in WorkflowUI, which excludes WorkflowHostingController & DescribedViewController. option 2. may require less code to implement, but adds indirection (swizzling is often not particularly discoverable), runtime overhead, and may not work for Swift-only methods.

note: this is all currently implemented behind 'experimental' SPI declarations. the intent would be to treat this as sort of an 'alpha' version of this functionality, and to set expectations that it may have breaking changes going forward, without a major version increment in the library.

Implementation

to achieve this goal, we've implemented an approach that does the following:

unifies all UIViewController subtypes defined in WorkflowUI under a new parent class WorkflowUIViewController
instruments some view controller lifecycle events (viewDidAppear, etc) within the shared base class
exposes a static value that can be used to register a 'global' observer of these events
adds some new protocols and event data types to enable observation of WorkflowUI lifecycle events

Usage Example

the envisioned consumer is something like a shared telemetry system that wishes to observe certain WorkflowUI events and directly use or attach metadata to them. for example, imagine a consumer wished to log an event whenever a ViewController from WorkflowUI appeared on screen. with the proposed changes, this could be accomplished as follows:

// implement an observer
struct AppearanceLogger: WorkflowUIObserver {
    var loggingSystem: LoggingSystem

    // handle all emitted observation events
    func observeEvent<E: WorkflowUIEvent>(_ event: E) {
        // perform specific observation logic
        if let appearanceEvent = event as? ViewDidAppearEvent {
            loggingSystem.logAppearanceEvent(appearanceEvent.viewController)
        }
    }
}

// install the global observer
WorkflowUIObservation.sharedUIObserver = AppearanceLogger()

Checklist

Unit Tests
UI Tests
Snapshot Tests (iOS only)
I have made corresponding changes to the documentation

jamieQ · 2023-06-06T20:15:40Z

WorkflowUI/Sources/Observation/WorkflowUIEvents.swift

+import Foundation
+import UIKit
+
+public protocol WorkflowUIEvent {


looking for feedback on the general structuring of the event types and related protocols

Structure looks good to me

WorkflowUI/Sources/Hosting/WorkflowHostingController.swift

jamieQ · 2023-06-06T20:25:30Z

WorkflowUI/Tests/WorkflowUIObservationTestCase.swift

+@_spi(WorkflowUIGlobalObservation) import WorkflowUI
+import XCTest
+
+open class WorkflowUIObservationTestCase: XCTestCase {


there's some scratch work in here, but some ideas for how one could tie into this system and forward events through Combine

WorkflowUI/Sources/Observation/WorkflowUIEvents.swift

WorkflowUI/Sources/Screen/ScreenViewController.swift

WorkflowUI/Sources/Observation/WorkflowUIViewController.swift

mjohnson12

This looks good to me and flows well with the rest of the WorkflowUI code.

Kiesco08

Approach lgtm too, seems easy to use

watt · 2023-06-12T18:00:01Z

WorkflowUI/Sources/Hosting/WorkflowHostingController.swift

-        super.viewDidLayoutSubviews()
+        defer { super.viewDidLayoutSubviews() }


Is this change needed for your hooks to work? We were considering moving the frame assignment to viewWillLayoutSubviews as part of the ViewEnvironmentUI feature, but wanted to isolate that change due to potential for side effects in code that was depending on this order of events.

no i don't think this change is necessary. the previous implementation of super.viewDidLayoutSubviews() did nothing (according to the docs), and the new one emits an event. it seems like the event emission should occur after any logic in this method, but i don't think the ordering relative to the frame assignment will matter to any of the envisioned consumers.

if we're potentially going to move the frame assignment out of this method, then perhaps we should leave this proposed change since it will preserve the relative ordering with the new observation events?

square-tomb

Looks good to me!

WorkflowSwiftUI contains a type WorkflowHostingViewController that should probably now subclass WorkflowUIViewController, too.

Other approaches to SwiftUI support that we're experimenting with involve a view controller type that sublclasses UIHostingController and so cannot subclass WorkflowUIViewController.

However, I think any view controller type like that can be modified to contain, rather than subclass, UIHostingController. The only downside AFAIK would be a deeper VC hierarchy.

n8chur

I'm a bit nervous about the reliance on a UIViewController superclass when ViewControllerDescription does not enforce this, but it seems like it may be worth the alternative given our goals!

ViewControllerDescriptions not describing a ScreenViewController subclass does seem like a potential footgun here (what @square-tomb pointed out in a previous comment).

Are we planning on adding documentation that describes in detail how to adhere to these event contract rules once it's out of alpha? Could Swift macros help make adding adherence easier in the future without needing to wrap one VC in another? It'd be nice to avoid needing to compose a nested VC to get this behavior.

WorkflowUI/Tests/WorkflowUIViewControllerTests.swift

WorkflowUI/Sources/Observation/WorkflowUIViewController.swift

kyleve · 2023-06-13T13:40:58Z

ViewControllerDescriptions not describing a ScreenViewController subclass does seem like a potential footgun here

Agreed. I don't think this is something we can/should enforce here, IMO.

square-tomb · 2023-06-13T14:12:27Z

I wonder, do we need events fromScreenViewController at all? Maybe the events from DescribedViewController are sufficient if that VC usually maps 1:1 to a screen.

If not, and DescribedViewController is often used to display a succession of different screens, we would have to add some events here. Or we could wrap currentViewController in a container WorkflowUIViewController if we don't mind deepening the VC hierarchy.

…lemetry * origin/main: [feat]: add ViewEnvironmentUI and support automatic bridging in WorkflowUI (#215) [chore]: update codeowners (#223) Add missing keyword 'func' to Tutorial1.md (#219)

jamieQ · 2023-06-13T20:28:49Z

WorkflowUI/Sources/Hosting/WorkflowHostingController.swift

@@ -22,7 +22,7 @@ import UIKit
 import Workflow

 /// Drives view controllers from a root Workflow.
-public final class WorkflowHostingController<ScreenType, Output>: UIViewController where ScreenType: Screen {
+public final class WorkflowHostingController<ScreenType, Output>: WorkflowUIViewController where ScreenType: Screen {


@square-tomb responding here for threading purposes:

WorkflowSwiftUI contains a type WorkflowHostingViewController that should probably now subclass WorkflowUIViewController, too.

good callouts, but given the current motivations for this interface, i think it's okay to punt on the story with SwiftUI and ignore this for the time being.

I wonder, do we need events fromScreenViewController at all? Maybe the events from DescribedViewController are sufficient if that VC usually maps 1:1 to a screen.

being able to recover the Screen type from ScreenViewController may be of use

If not, and DescribedViewController is often used to display a succession of different screens, we would have to add some events here. Or we could wrap currentViewController in a container WorkflowUIViewController if we don't mind deepening the VC hierarchy.

adding some additional events that are more specific to the logic in some of the VC subtypes may be something we want to do eventually, but for now i thought we'd just start with VC lifecycle information and see how far that gets us.

jamieQ · 2023-06-13T20:34:52Z

WorkflowUI/Sources/Hosting/WorkflowHostingController.swift

@@ -22,7 +22,7 @@ import UIKit
 import Workflow

 /// Drives view controllers from a root Workflow.
-public final class WorkflowHostingController<ScreenType, Output>: UIViewController where ScreenType: Screen {
+public final class WorkflowHostingController<ScreenType, Output>: WorkflowUIViewController where ScreenType: Screen {


@n8chur responding here for threading purposes:

I'm a bit nervous about the reliance on a UIViewController superclass when ViewControllerDescription does not enforce this, but it seems like it may be worth the alternative given our goals!

that's a reasonable point, but i think we can revisit this if it ends up being problematic in practice.

ViewControllerDescriptions not describing a ScreenViewController subclass does seem like a potential footgun here (what @square-tomb pointed out in a previous comment).

that's true, though i'm not sure if that will pose an issue for the initial use of this information. @amorde any concerns with this?

Are we planning on adding documentation that describes in detail how to adhere to these event contract rules once it's out of alpha?

yep, this is a first pass so i have low conviction on what exactly the 'right' or desired contract is.

Could Swift macros help make adding adherence easier in the future without needing to wrap one VC in another?

possibly, though i don't really have a sense of how we could concretely leverage that tool at the moment. i thought macros had to be additive, so i'm not sure we'd be able to change any existing code in client-supplied VCs.

i thought macros had to be additive, so i'm not sure we'd be able to change any existing code in client-supplied VCs.

I dove a little deeper into Swift Macros after my initial comment and I think you're right, it looks like it may not be useful here.

jamieQ · 2023-06-13T20:43:47Z

WorkflowUI/Sources/Hosting/WorkflowHostingController.swift

@@ -22,7 +22,7 @@ import UIKit
 import Workflow

 /// Drives view controllers from a root Workflow.
-public final class WorkflowHostingController<ScreenType, Output>: UIViewController where ScreenType: Screen {
+public final class WorkflowHostingController<ScreenType, Output>: WorkflowUIViewController where ScreenType: Screen {


@kyleve responding here for threading purposes:

ViewControllerDescriptions not describing a ScreenViewController subclass does seem like a potential footgun here

Agreed. I don't think this is something we can/should enforce here, IMO.

could you clarify – you think this is a risk, but not one we should change the contract of ViewControllerDescription to deal with?

jamieQ added 2 commits May 26, 2023 10:02

[WIP]: UI observation draft

451e3ed

refactoring & tests

28fe9cd

jamieQ commented Jun 6, 2023

View reviewed changes

WorkflowUI/Sources/Hosting/WorkflowHostingController.swift Outdated Show resolved Hide resolved

jamieQ commented Jun 6, 2023

View reviewed changes

amorde approved these changes Jun 8, 2023

View reviewed changes

WorkflowUI/Sources/Observation/WorkflowUIEvents.swift Outdated Show resolved Hide resolved

WorkflowUI/Sources/Screen/ScreenViewController.swift Outdated Show resolved Hide resolved

WorkflowUI/Sources/Observation/WorkflowUIViewController.swift Outdated Show resolved Hide resolved

refactor to just the VC lifecycle events, make things SPI, add tests

c9289c2

jamieQ marked this pull request as ready for review June 12, 2023 16:49

jamieQ requested review from a team as code owners June 12, 2023 16:49

jamieQ commented Jun 12, 2023

View reviewed changes

WorkflowUI/Sources/Observation/WorkflowUIViewController.swift Show resolved Hide resolved

mjohnson12 approved these changes Jun 12, 2023

View reviewed changes

Kiesco08 approved these changes Jun 12, 2023

View reviewed changes

watt reviewed Jun 12, 2023

View reviewed changes

square-tomb approved these changes Jun 12, 2023

View reviewed changes

n8chur approved these changes Jun 12, 2023

View reviewed changes

WorkflowUI/Tests/WorkflowUIViewControllerTests.swift Outdated Show resolved Hide resolved

WorkflowUI/Sources/Observation/WorkflowUIViewController.swift Show resolved Hide resolved

jamieQ added 3 commits June 13, 2023 14:48

pr feedback & cleanup

dd69582

Merge remote-tracking branch 'origin/main' into jquadri/workflowui-te…

9b898e4

…lemetry * origin/main: [feat]: add ViewEnvironmentUI and support automatic bridging in WorkflowUI (#215) [chore]: update codeowners (#223) Add missing keyword 'func' to Tutorial1.md (#219)

remove some merge noise

a0d88e9

jamieQ commented Jun 13, 2023

View reviewed changes

cleanup & docs

21a7237

jamieQ changed the title ~~[WIP]: WorkflowUI telemetry hooks~~ [feat]: add experimental WorkflowUI telemetry hooks Jun 14, 2023

jamieQ added 2 commits June 14, 2023 10:25

attempt to fix SPM job test failures

2db6b33

try to fix macOS tests

1db7247

jamieQ merged commit 5010727 into main Jun 14, 2023

jamieQ deleted the jquadri/workflowui-telemetry branch June 14, 2023 16:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feat]: add experimental WorkflowUI telemetry hooks #221

[feat]: add experimental WorkflowUI telemetry hooks #221

jamieQ commented Jun 2, 2023 •

edited

Loading

jamieQ Jun 6, 2023

amorde Jun 12, 2023

jamieQ Jun 6, 2023

mjohnson12 left a comment

Kiesco08 left a comment

watt Jun 12, 2023

jamieQ Jun 12, 2023

square-tomb left a comment

n8chur left a comment •

edited

Loading

kyleve commented Jun 13, 2023

square-tomb commented Jun 13, 2023

jamieQ Jun 13, 2023

jamieQ Jun 13, 2023

jamieQ Jun 13, 2023 •

edited

Loading

n8chur Jun 13, 2023

jamieQ Jun 13, 2023

		super.viewDidLayoutSubviews()
		defer { super.viewDidLayoutSubviews() }

[feat]: add experimental WorkflowUI telemetry hooks #221

[feat]: add experimental WorkflowUI telemetry hooks #221

Conversation

jamieQ commented Jun 2, 2023 • edited Loading

Motivation

Implementation

Usage Example

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjohnson12 left a comment

Choose a reason for hiding this comment

Kiesco08 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

square-tomb left a comment

Choose a reason for hiding this comment

n8chur left a comment • edited Loading

Choose a reason for hiding this comment

kyleve commented Jun 13, 2023

square-tomb commented Jun 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamieQ Jun 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamieQ commented Jun 2, 2023 •

edited

Loading

n8chur left a comment •

edited

Loading

jamieQ Jun 13, 2023 •

edited

Loading